Update resource_aws_sns_topic_subscription to create the subscription… #10496

carlos-zaragoza · 2019-10-13T21:23:08Z

… attributes simulatenously with the general subscription creation

Previously the attributes were created after the subscription was created. This left a few seconds in time where messages from the topic could make it through to the endpoint without adhering to things like the FilterPolicy and the RawMessageDelivery flag.

Community Note

Please vote on this pull request by adding a 👍 reaction to the original pull request comment to help the community and maintainers prioritize this request
Please do not leave "+1" comments, they generate extra noise for pull request followers and do not help prioritize the request

Relates OR Closes #10495

Release note for CHANGELOG:

SNS Subscription attributes (`raw_message_delivery`, `filter_policy`, and `delivery_policy`) will now be created at the same time the subscription is created rather than afterwards.

Output from acceptance testing:

When I run the tests one at a time, they pass successfully. The tests also pass when run concurrently on the automated build server.

I have issues running the tests concurrently on my machine but the issue happens both on this branch and on master so I believe this issue is a problem with my local environment.

For completeness' sake, I've included the test results of running both concurrently and running one at a time. They are listed separately below.

$ make testacc TEST=./aws TESTARGS='-run=TestAccAWSSNSTopicSubscription'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -count 1 -parallel 40 -run=TestAccAWSSNSTopicSubscription -timeout 120m
=== RUN   TestAccAWSSNSTopicSubscription_basic
=== PAUSE TestAccAWSSNSTopicSubscription_basic
=== RUN   TestAccAWSSNSTopicSubscription_filterPolicy
=== PAUSE TestAccAWSSNSTopicSubscription_filterPolicy
=== RUN   TestAccAWSSNSTopicSubscription_deliveryPolicy
=== PAUSE TestAccAWSSNSTopicSubscription_deliveryPolicy
=== RUN   TestAccAWSSNSTopicSubscription_rawMessageDelivery
=== PAUSE TestAccAWSSNSTopicSubscription_rawMessageDelivery
=== RUN   TestAccAWSSNSTopicSubscription_autoConfirmingEndpoint
=== PAUSE TestAccAWSSNSTopicSubscription_autoConfirmingEndpoint
=== RUN   TestAccAWSSNSTopicSubscription_autoConfirmingSecuredEndpoint
=== PAUSE TestAccAWSSNSTopicSubscription_autoConfirmingSecuredEndpoint
=== CONT  TestAccAWSSNSTopicSubscription_basic
=== CONT  TestAccAWSSNSTopicSubscription_autoConfirmingEndpoint
=== CONT  TestAccAWSSNSTopicSubscription_deliveryPolicy
=== CONT  TestAccAWSSNSTopicSubscription_rawMessageDelivery
=== CONT  TestAccAWSSNSTopicSubscription_autoConfirmingSecuredEndpoint
=== CONT  TestAccAWSSNSTopicSubscription_filterPolicy
--- FAIL: TestAccAWSSNSTopicSubscription_filterPolicy (14.31s)
    testing.go:569: Step 0 error: Check failed: Check 2/2 error: aws_sns_topic_subscription.test_subscription: Attribute 'filter_policy' expected "{\"key1\": [\"val1\"], \"key2\": [\"val2\"]}", got ""
    testing.go:630: Error destroying resource! WARNING: Dangling resources
        may exist. The full state and error is shown below.

        Error: errors during apply: AWS.SimpleQueueService.NonExistentQueue: The specified queue does not exist for this wsdl version.
                status code: 400, request id: 832a200f-a5f1-58b1-b128-76122e3a8a2e

        State: aws_sqs_queue.test_queue:
          ID = https://sqs.us-west-2.amazonaws.com/072662658510/terraform-subscription-test-queue-9103664681323863048
          provider = provider.aws
          arn = arn:aws:sqs:us-west-2:072662658510:terraform-subscription-test-queue-9103664681323863048
          content_based_deduplication = false
          delay_seconds = 0
          fifo_queue = false
          kms_data_key_reuse_period_seconds = 300
          kms_master_key_id =
          max_message_size = 262144
          message_retention_seconds = 345600
          name = terraform-subscription-test-queue-9103664681323863048
          policy =
          receive_wait_time_seconds = 0
          redrive_policy =
          tags.% = 0
          visibility_timeout_seconds = 30
--- FAIL: TestAccAWSSNSTopicSubscription_rawMessageDelivery (14.41s)
    testing.go:569: Step 0 error: Check failed: Check 2/2 error: aws_sns_topic_subscription.test_subscription: Attribute 'raw_message_delivery' expected "true", got "false"
--- FAIL: TestAccAWSSNSTopicSubscription_basic (14.41s)
    testing.go:569: Step 0 error: Check failed: Check 3/8 error: aws_sns_topic_subscription.test_subscription: Attribute 'delivery_policy' expected "", got "{\"healthyRetryPolicy\":{\"minDelayTarget\":5,\"maxDelayTarget\":20,\"numRetries\":5,\"numMaxDelayRetries\":null,\"numNoDelayRetries\":null,\"numMinDelayRetries\":null,\"backoffFunction\":null},\"sicklyRetryPolicy\":null,\"throttlePolicy\":null,\"guaranteed\":false}"
--- FAIL: TestAccAWSSNSTopicSubscription_autoConfirmingSecuredEndpoint (19.37s)
    testing.go:569: Step 0 error: errors during apply:

        Error: Error creating IAM Role tf-acc-test-sns-9103664681323863048: EntityAlreadyExists: Role with name tf-acc-test-sns-9103664681323863048 already exists.
                status code: 409, request id: 7083aa00-689b-4fb8-94cd-3857002a45a0

          on C:\Users\socce\AppData\Local\Temp\tf-test576338621\main.tf line 51:
          (source code not available)


--- FAIL: TestAccAWSSNSTopicSubscription_autoConfirmingEndpoint (51.90s)
    testing.go:569: Step 0 error: errors during apply:

        Error: Error creating SNS topic: NotFound: Topic does not exist
                status code: 404, request id: b9a48349-709f-5d2b-af55-d554fb452784

          on C:\Users\socce\AppData\Local\Temp\tf-test233225962\main.tf line 113:
          (source code not available)


--- PASS: TestAccAWSSNSTopicSubscription_deliveryPolicy (111.66s)
FAIL
FAIL    github.com/terraform-providers/terraform-provider-aws/aws       112.014s
FAIL
make: *** [GNUmakefile:24: testacc] Error 1

Results of running all the tests one at a time.

$ make testacc TEST=./aws TESTARGS='-run=TestAccAWSSNSTopicSubscription_basic'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -count 1 -parallel 20 -run=TestAccAWSSNSTopicSubscription_basic -timeout 120m
=== RUN   TestAccAWSSNSTopicSubscription_basic
=== PAUSE TestAccAWSSNSTopicSubscription_basic
=== CONT  TestAccAWSSNSTopicSubscription_basic
--- PASS: TestAccAWSSNSTopicSubscription_basic (47.87s)
PASS
ok      github.com/terraform-providers/terraform-provider-aws/aws       48.291s


$ make testacc TEST=./aws TESTARGS='-run=TestAccAWSSNSTopicSubscription_filterPolicy'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -count 1 -parallel 20 -run=TestAccAWSSNSTopicSubscription_filterPolicy -timeout 120m
=== RUN   TestAccAWSSNSTopicSubscription_filterPolicy
=== PAUSE TestAccAWSSNSTopicSubscription_filterPolicy
=== CONT  TestAccAWSSNSTopicSubscription_filterPolicy
--- PASS: TestAccAWSSNSTopicSubscription_filterPolicy (59.27s)
PASS
ok      github.com/terraform-providers/terraform-provider-aws/aws       59.608s


$ make testacc TEST=./aws TESTARGS='-run=TestAccAWSSNSTopicSubscription_deliveryPolicy'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -count 1 -parallel 20 -run=TestAccAWSSNSTopicSubscription_deliveryPolicy -timeout 120m
=== RUN   TestAccAWSSNSTopicSubscription_deliveryPolicy
=== PAUSE TestAccAWSSNSTopicSubscription_deliveryPolicy
=== CONT  TestAccAWSSNSTopicSubscription_deliveryPolicy
--- PASS: TestAccAWSSNSTopicSubscription_deliveryPolicy (53.87s)
PASS
ok      github.com/terraform-providers/terraform-provider-aws/aws       54.091s


$ make testacc TEST=./aws TESTARGS='-run=TestAccAWSSNSTopicSubscription_rawMessageDelivery'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -count 1 -parallel 20 -run=TestAccAWSSNSTopicSubscription_rawMessageDelivery -timeout 120m
=== RUN   TestAccAWSSNSTopicSubscription_rawMessageDelivery
=== PAUSE TestAccAWSSNSTopicSubscription_rawMessageDelivery
=== CONT  TestAccAWSSNSTopicSubscription_rawMessageDelivery
--- PASS: TestAccAWSSNSTopicSubscription_rawMessageDelivery (61.36s)
PASS
ok      github.com/terraform-providers/terraform-provider-aws/aws       61.681s


$ make testacc TEST=./aws TESTARGS='-run=TestAccAWSSNSTopicSubscription_autoConfirmingEndpoint'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -count 1 -parallel 20 -run=TestAccAWSSNSTopicSubscription_autoConfirmingEndpoint -timeout 120m
=== RUN   TestAccAWSSNSTopicSubscription_autoConfirmingEndpoint
=== PAUSE TestAccAWSSNSTopicSubscription_autoConfirmingEndpoint
=== CONT  TestAccAWSSNSTopicSubscription_autoConfirmingEndpoint
--- PASS: TestAccAWSSNSTopicSubscription_autoConfirmingEndpoint (46.61s)
PASS
ok      github.com/terraform-providers/terraform-provider-aws/aws       46.825s


$ make testacc TEST=./aws TESTARGS='-run=TestAccAWSSNSTopicSubscription_autoConfirmingSecuredEndpoint'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -count 1 -parallel 20 -run=TestAccAWSSNSTopicSubscription_autoConfirmingSecuredEndpoint -timeout 120m
=== RUN   TestAccAWSSNSTopicSubscription_autoConfirmingSecuredEndpoint
=== PAUSE TestAccAWSSNSTopicSubscription_autoConfirmingSecuredEndpoint
=== CONT  TestAccAWSSNSTopicSubscription_autoConfirmingSecuredEndpoint
--- PASS: TestAccAWSSNSTopicSubscription_autoConfirmingSecuredEndpoint (111.10s)
PASS
ok      github.com/terraform-providers/terraform-provider-aws/aws       111.425s

… attributes simulatenously with the general subscription creation Previously the attributes were created _after_ the subscription was created. This left a few seconds in time where messages from the topic could make it through to the endpoint without adhering to things like the FilterPolicy and the RawMessageDelivery flag.

carlos-zaragoza · 2019-10-13T21:27:33Z

I believe the issues I faced running the tests in parallel were localized to my machine. The tests passed successfully on the automated build server.

mjgpy3 · 2020-01-27T17:51:11Z

If I'm understand this PR correctly, the flow (before these changes) looked like this

So that's 4 separate requests to subscribe and create subscription attributes.

The idea then is that, in high traffic environments, messages might be delivered before the requests for limiting attributes (e.g. filter_policy) go through. Is that correct @carlos-zaragoza?

This PR just simplifies the creation-path to the following

which should subscribe and create the limiting attributes atomically.

Is there still an issue (with atomic updates) when just an update occurs since the update-path still makes multiple calls?

mjgpy3 · 2020-01-27T17:58:18Z

Moreover, it's hard to say how long these requests actually take since all kinds of "eventually consistency" is at play here.

carlos-zaragoza · 2020-02-04T17:20:24Z

@mjgpy3 Yes, your explanation and diagrams are correct. It's all about doing the request once with exactly what the user wants rather than pieces of what they want until it finally comes together.

You bring up a good point about the update-path that I hadn't noticed. As it is currently, it could potentially cause problems if someone needed more than one attribute changed at the same time. That should probably be changed as well.

carlos-zaragoza · 2020-02-15T20:31:13Z

@mjgpy3 I got around to looking into the update-path question and it looks like it has to be done in multiple calls. The set-subscription-attributes command doesn't support updating multiple attributes at once.

So I think the changes as they are now is about as good as the current CLI can do.

mjgpy3 · 2020-02-15T20:58:14Z

Thanks for looking into that, @carlos-zaragoza.

mike-dazn · 2020-03-24T12:59:51Z

Hi, Has there been any progress on getting this change in? We're having the same issues as #10495

carlos-zaragoza · 2020-03-26T00:04:35Z

@mike-dazn I've yet to hear from the maintainers presumably because the issue and the PR are too far down the list when sorting by community reactions (👍). Please make sure to leave a 👍 reaction on both to bump them higher in the priority list if this change would help you.

carlos-zaragoza · 2020-07-16T03:04:10Z

Hi can we get some sort of update on this getting through? It's been quite a while.

AdamSlack · 2020-07-28T13:52:59Z

Just want to add some more noise to this, would be great to see this make some progress! Would be very useful for us.

bflad

Apologies for the delayed review, @carlos-zaragoza, this looks good to me. 🚀

Output from acceptance testing in AWS Commercial:

--- PASS: TestAccAWSSNSTopicSubscription_basic (18.88s)
--- PASS: TestAccAWSSNSTopicSubscription_filterPolicy (42.59s)
--- PASS: TestAccAWSSNSTopicSubscription_rawMessageDelivery (43.36s)
--- PASS: TestAccAWSSNSTopicSubscription_deliveryPolicy (43.50s)
--- PASS: TestAccAWSSNSTopicSubscription_autoConfirmingEndpoint (46.81s)
--- PASS: TestAccAWSSNSTopicSubscription_autoConfirmingSecuredEndpoint (86.36s)

Output from acceptance testing in AWS GovCloud (US) (test failures unrelated due to API Gateway setup in testing configuration):

--- PASS: TestAccAWSSNSTopicSubscription_basic (17.55s)
--- FAIL: TestAccAWSSNSTopicSubscription_autoConfirmingEndpoint (20.69s)
--- FAIL: TestAccAWSSNSTopicSubscription_autoConfirmingSecuredEndpoint (27.48s)
--- PASS: TestAccAWSSNSTopicSubscription_filterPolicy (35.59s)
--- PASS: TestAccAWSSNSTopicSubscription_rawMessageDelivery (36.02s)
--- PASS: TestAccAWSSNSTopicSubscription_deliveryPolicy (36.23s)

ghost · 2020-10-09T16:19:19Z

This has been released in version 3.10.0 of the Terraform AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template for triage. Thanks!

ghost · 2020-11-02T17:10:25Z

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks!

ghost added size/S Managed by automation to categorize the size of a PR. service/sns Issues and PRs that pertain to the sns service. labels Oct 13, 2019

carlos-zaragoza marked this pull request as ready for review October 13, 2019 21:25

carlos-zaragoza requested a review from a team October 13, 2019 21:25

Simplify conditional

5823502

carlos-zaragoza mentioned this pull request Nov 4, 2019

SNS subscription filter applied too late after subscription creation #10495

Closed

anGie44 force-pushed the master branch from 6c6566f to 081bf57 Compare June 29, 2020 16:14

DrFaust92 mentioned this pull request Jul 9, 2020

r/sns_topic_subscription - recreate on topic deletion + validations #14101

Merged

bflad added the enhancement Requests to existing resources that expand the functionality or scope. label Oct 1, 2020

bflad approved these changes Oct 2, 2020

View reviewed changes

bflad added this to the v3.10.0 milestone Oct 2, 2020

bflad merged commit 7cb55a8 into hashicorp:master Oct 2, 2020

bflad added a commit that referenced this pull request Oct 2, 2020

Update CHANGELOG for #10496

d28d475

carlos-zaragoza deleted the AddSubscriptionFilterOnCreate branch October 5, 2020 14:10

ghost locked as resolved and limited conversation to collaborators Nov 2, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update resource_aws_sns_topic_subscription to create the subscription… #10496

Update resource_aws_sns_topic_subscription to create the subscription… #10496

carlos-zaragoza commented Oct 13, 2019 •

edited

Loading

carlos-zaragoza commented Oct 13, 2019 •

edited

Loading

mjgpy3 commented Jan 27, 2020

mjgpy3 commented Jan 27, 2020

carlos-zaragoza commented Feb 4, 2020 •

edited

Loading

carlos-zaragoza commented Feb 15, 2020

mjgpy3 commented Feb 15, 2020

mike-dazn commented Mar 24, 2020

carlos-zaragoza commented Mar 26, 2020

carlos-zaragoza commented Jul 16, 2020

AdamSlack commented Jul 28, 2020

bflad left a comment

ghost commented Oct 9, 2020

ghost commented Nov 2, 2020

Update resource_aws_sns_topic_subscription to create the subscription… #10496

Update resource_aws_sns_topic_subscription to create the subscription… #10496

Conversation

carlos-zaragoza commented Oct 13, 2019 • edited Loading

Community Note

Output from acceptance testing:

carlos-zaragoza commented Oct 13, 2019 • edited Loading

mjgpy3 commented Jan 27, 2020

mjgpy3 commented Jan 27, 2020

carlos-zaragoza commented Feb 4, 2020 • edited Loading

carlos-zaragoza commented Feb 15, 2020

mjgpy3 commented Feb 15, 2020

mike-dazn commented Mar 24, 2020

carlos-zaragoza commented Mar 26, 2020

carlos-zaragoza commented Jul 16, 2020

AdamSlack commented Jul 28, 2020

bflad left a comment

Choose a reason for hiding this comment

ghost commented Oct 9, 2020

ghost commented Nov 2, 2020

carlos-zaragoza commented Oct 13, 2019 •

edited

Loading

carlos-zaragoza commented Oct 13, 2019 •

edited

Loading

carlos-zaragoza commented Feb 4, 2020 •

edited

Loading